-
Notifications
You must be signed in to change notification settings - Fork 749
refactor(apprunner): migrate to sdkv3. #6764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9d18128 to
88f7649
Compare
88f7649 to
2827c0f
Compare
| const subform = new WizardForm<AppRunner.CodeRepository>() | ||
| function createCodeRepositorySubForm(git: GitExtension): WizardForm<AppRunnerCodeRepository> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for future reference: the AppRunner.foo was intentional, to avoid needing to import everything explicitly. is that not possible with the new sdk? it's useful for discovery, e.g. one can do AppRunner.<tab> to complete other members, and also avoids noise in the imports list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe SDKv3 exports a namespace containing its corresponding types. It seems this is done intentionally to reduce bundle-size. (source: https://aws.amazon.com/blogs/developer/modular-packages-in-aws-sdk-for-javascript/). We could alternatively import the whole package and take the trade off of losing potential tree shaking benefits in favor of readability with something like:
import * as AppRunner from '@aws-sdk/client-apprunner'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could alternatively import the whole package and take the trade off of losing potential tree shaking benefits in favor of readability with something like:
That is the usual pattern, yes. I didn't know it could interfere with tree-shaking... do you have a reference I can read about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call out.
I was confused because in the article I linked they say
The v3 also lets you import a modular bare-bones client with specific commands that help further reduce application bundle size!
However, this only applies to the CommonJS require syntax since it resolves dynamically. My understanding is that since ES Module is resolved statically, webpack can drop the unused pieces as dependencies in the bundle regardless of which import style we use.
(based on discussions here: https://stackoverflow.com/questions/46677752/the-difference-between-requirex-and-import-x and https://webpack.js.org/guides/tree-shaking/)
| SourceCodeVersion: RequiredProps<SourceCodeVersion, 'Type' | 'Value'> | ||
| CodeConfiguration: AppRunnerCodeConfiguration | ||
| } | ||
| export interface AppRunnerSourceConfiguration extends SourceConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not bother with the AppRunnerSource prefix. And there would be no conflict if we did it like this:
| export interface AppRunnerSourceConfiguration extends SourceConfiguration { | |
| export interface SourceConfiguration extends AppRunner.SourceConfiguration { |
It's probably important to fix this, or at least for any remaining migrations, because people copy whatever is already being done in clients. I don't think we want to add prefixes to every type name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree its not a great solution. One alternative would be to shadow the names of the underlying types, and import them as different names. Ex.
import { SourceConfiguration as SourceConfigurationSDK } from '@aws-sdk/client-apprunner'
...
export interface SourceConfiguration extends SourceConfigurationSDK {
My main reason for not doing this is that it could create confusion in cases where the custom type doesn't completely mirror the sdk type. As an example, in EC2 we add a LastSeenStatus field to the wrapping type that doesn't exist on the sdk type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could create confusion in cases where the custom type doesn't completely mirror the sdk type.
The types are coming from this module, thus developers must understand they aren't the same. They have the same names, but developers are expected to understand that any module can use conflicting names anywhere. If one imports Foo from module A, that's unrelated to Foo from module B, even though they have the same name.
And in this codebase, the sdk wrappers/clients are intentionally imperfect projections of the sdk types. That is a fact of life for any application that wraps things.
LMK if I misunderstood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, and seems in-line with how we construct the clients. I'll adjust the names of the types in this PR and look into a followup to refactor the existing work done.
|
Could merge today since it doesn't affect Q. |
|
/runIntegrationTests |
Problem
app runner still uses sdkv2.
Solution
getServiceSummariesinAppRunnerNodeto use SDK native pagination. This includes updates to the tests forAppRunnerNode.Verification
feature/xbranches will not be squash-merged at release time.